You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In an effort for Selenium's raw BiDi to return the exact response from the endpoint, we should not throw exceptions unless the opt-in ThrowOnError() method is called.
Here are some key observations to aid the review process:
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review
Type Safety The ThrowOnError method assumes that if a result is not EvaluateResultSuccess, it must be EvaluateResultException. This could lead to runtime errors if new result types are added in the future.
The Message property now joins stack trace frames with newlines, but there's no validation that CallFrames is not null or empty, which could lead to unexpected formatting.
The code assumes that _evaluateResultException.ExceptionDetails.StackTrace is always non-null, but it could be null in some error scenarios. Add a null check to prevent a potential NullReferenceException when formatting the error message.
Why: This is a valid defensive programming suggestion that could prevent potential NullReferenceExceptions if StackTrace is null. Adding null checking is a good practice for error handling code, especially in exception formatting.
Medium
General
Fix property visibility issue
The Type property is marked as internal which might prevent proper JSON serialization in some contexts. Since this is a property that needs to be included in serialization (as indicated by the comment and JsonInclude attribute), it should be made public to ensure consistent serialization behavior.
public record ChannelLocalValue(ChannelProperties Value) : LocalValue
{
// AddPreloadScript takes arguments typed as ChannelLocalValue but still requires "type":"channel"
[JsonInclude]
[JsonPropertyName("type")]
- internal string Type => "channel";+ public string Type => "channel";
}
[To ensure code accuracy, apply this suggestion manually]
Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies that changing the visibility of the Type property from internal to public could improve serialization consistency. This is a reasonable suggestion for better API design, though the current implementation likely works in most scenarios.
Low
Learned best practice
✅ Use safe type checking with the 'as' operator instead of direct casting to prevent potential InvalidCastExceptionSuggestion Impact:The commit addressed the same issue as the suggestion but implemented a different solution. Instead of using 'as' operator with null checking, the method was renamed to 'AsSuccess' and still uses direct casting but with a different approach to handle the exception case.
code diff:
- public EvaluateResultSuccess ThrowOnError()+ public EvaluateResultSuccess AsSuccess()
{
if (this is EvaluateResultSuccess success)
{
return success;
}
- throw new ScriptEvaluateException((EvaluateResultException)this);+ var exceptionResult = (EvaluateResultException)this;++ throw new ScriptEvaluateException(exceptionResult.ExceptionDetails);
The current implementation of ThrowOnError() uses a direct cast to EvaluateResultException without checking if the object is actually of that type. If this is neither EvaluateResultSuccess nor EvaluateResultException, it will throw an InvalidCastException. Use the as operator with null checking to make the code more robust and prevent potential runtime exceptions.
public EvaluateResultSuccess ThrowOnError()
{
if (this is EvaluateResultSuccess success)
{
return success;
}
- throw new ScriptEvaluateException((EvaluateResultException)this);+ var exception = this as EvaluateResultException;+ if (exception != null)+ {+ throw new ScriptEvaluateException(exception);+ }++ throw new InvalidOperationException($"Unexpected result type: {this.GetType().Name}");
}
RenderMichael
changed the title
[dotnet] [bidi] Do not throw when CallFunction or Evaluate return exc…
[dotnet] [bidi] Do not throw when CallFunction or Evaluate return exceptional result
Mar 28, 2025
nvborisenko
changed the title
[dotnet] [bidi] Do not throw when CallFunction or Evaluate return exceptional result
[dotnet] [bidi] Do not throw when CallFunction or Evaluate return exceptional result (breaking change)
Apr 2, 2025
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RenderMichael
changed the title
nvborisenko
changed the title
Description
In an effort for Selenium's raw BiDi to return the exact response from the endpoint, we should not throw exceptions unless the opt-in
ThrowOnError()method is called.Motivation and Context
Fixes #15414
Types of changes
Checklist
PR Type
Enhancement, Tests
Description
Updated
EvaluateAsyncandCallFunctionAsyncmethods to handle exceptional results without throwing exceptions directly.ThrowOnErrormethod inEvaluateResultto centralize error handling.EvaluateAsyncandCallFunctionAsynctoEvaluateResultfor consistency.Enhanced error handling by refining
ScriptEvaluateExceptionto include detailed stack trace information.Adjusted test cases to validate the new
ThrowOnErrorbehavior and ensure proper handling of exceptional results.EvaluateResultSuccessand useThrowOnErrorfor result validation.Improved serialization logic in
ChannelLocalValueto ensure proper JSON property naming.Changes walkthrough 📝
5 files
RefactoredEvaluateAsyncandCallFunctionAsyncmethods for errorhandling
AddedThrowOnErrormethod toEvaluateResultfor centralized errorhandling
Improved JSON serialization for `ChannelLocalValue`Enhanced exception message with detailed stack traceRemoved direct exception throwing in
EvaluateAsyncandCallFunctionAsync5 files
Updated tests to validateThrowOnErrorbehavior forCallFunctionAsyncAdjusted parameter tests to handleThrowOnErrorinCallFunctionAsyncRefined remote value tests to useThrowOnErrorfor result validationEnhanced evaluation parameter tests with `ThrowOnError` validationUpdated script command tests to validate `ThrowOnError` behavior